Floor DateTime truncation for pre-epoch timestamps - #3002
Conversation
DateTime::truncate and the into_timestamp_* helpers relied on plain integer division, which rounds towards zero. For timestamps before 1970 that rounds a value up in time, so an instant could be truncated into the following bucket and two instants inside the same second could end up in different buckets. truncate() is what builds the indexed term and the fast field value for date fields (at second precision), so pre-1970 dates with a sub-second component were stored under the wrong second and could be missed by exact and range queries. Use Euclidean division so truncation always floors to the start of the containing bucket, matching the behaviour already seen for positive timestamps.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd55312233
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| DateTimePrecision::Microseconds => (self.timestamp_nanos / 1_000) * 1_000, | ||
| let truncated_timestamp_nanos = match precision { | ||
| DateTimePrecision::Seconds => { | ||
| self.timestamp_nanos.div_euclid(1_000_000_000) * 1_000_000_000 |
There was a problem hiding this comment.
When truncating timestamps in the first partial bucket above DateTime::MIN (for example, DateTime::MIN.truncate(DateTimePrecision::Seconds)), the Euclidean quotient is one lower and multiplying it back produces a value below i64::MIN. In debug builds this panics; in typical release builds it wraps to a far-future positive timestamp, so indexing or searching with a lower-bound date can write the wrong term/fast-field value. Please saturate or special-case floor buckets that are outside the representable DateTime range.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eb1624daf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// Convert to UNIX timestamp in microseconds. | ||
| pub const fn into_timestamp_micros(self) -> i64 { | ||
| self.timestamp_nanos / 1_000 | ||
| self.timestamp_nanos.div_euclid(1_000) |
There was a problem hiding this comment.
For DateTime values in the first partial microsecond bucket above i64::MIN (for example DateTime::MIN), this floor division returns -9223372036854776, whose corresponding nanosecond value is below i64::MIN. Since BinarySerializable::serialize uses into_timestamp_micros() and deserialization calls from_timestamp_micros(), round-tripping such a valid DateTime now panics in debug builds or wraps to a far-future timestamp in release builds; the previous toward-zero quotient stayed within the constructible range.
Useful? React with 👍 / 👎.
1 participant
DateTime::truncateand theinto_timestamp_secs/millis/microshelpers used toward-zero integer division (/) where floor division is required. For a pre-1970 (negative) timestamp with a sub-second remainder,/rounds toward zero — i.e. forward in time — so an instant is truncated into the following bucket, and two instants inside the same wall-clock second land in different buckets.truncate(DATE_TIME_PRECISION_INDEXED)(seconds) builds the indexed term and fast-field value for date fields (from_field_date_for_search, the segment writer, JSON utils, the fast-field writer), so any indexed date before 1970 with a sub-second component is stored under the wrong second and can be missed by exact-match/range date queries. Positive timestamps are unaffected.Switched to Euclidean (floor) division (
div_euclid), so truncation always maps an instant to the start of its containing bucket. Added regression tests incommon/src/datetime.rsasserting truncation never moves a timestamp forward and that same-second instants co-bucket (positives unchanged).Note: a similar toward-zero division exists in the aggregation
date_histogrampath (a separate code path that dividesOffsetDateTime::unix_timestamp_nanos()directly, not viaDateTime). I've kept this PR scoped to theDateTimeindexing path; happy to address the aggregation case in a follow-up.